test!: attempt to resolve flakes #2834

rootulp · 2023-11-10T13:04:00Z

Attempt to close #2672.

This PR refactors some of the tests. It also removes the _Flaky suffix from other tests that I can't locally reproduce failures for. I plan on keeping the other "flake" issues open for now.

Potential solution for https://github.com/celestiaorg/celestia-app/actions/runs/6827269518/job/18569018708#step:4:49

rootulp · 2023-11-25T02:07:38Z

app/test/max_total_blob_size_test.go

 		{
 			name: "2 mebibyte blob",
-			blob: mustNewBlob(2 * mebibyte),
+			blob: newBlobWithSize(2 * mebibyte),
 			want: blobtypes.ErrTotalBlobSizeTooLarge.ABCICode(),
 		},


It seems like the multiple test cases of this test are affecting each other. Since the first two test cases aren't expected to encounter an error and that happy path is covered in other tests, proposal to drop them from this test.

coderabbitai · 2023-11-25T02:33:03Z

Warning

Rate Limit Exceeded

@rootulp has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 7 minutes and 34 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between 3e0c902 and 4a9d8e5.

Walkthrough

The changes across various test files and packages reflect a refactoring and stabilization effort. The modifications include renaming test functions for clarity, removing flakiness indicators from test names, updating test structures and methods to accommodate new testing patterns, and refining test cases to focus on specific behaviors. Additionally, there's an introduction of new utility functions and constants to streamline testing processes. The changes aim to improve the reliability and readability of the test suite, ensuring that tests are more deterministic and better aligned with the intended test scenarios.

Changes

File Path	Change Summary
`app/test/.../block_production_test.go`	Changed package name from "app" to "app_test" in import statement.
`app/test/.../integration_test.go`	Updated `IntegrationTestSuite` struct and methods, added new blob size tests, and removed `TestSubmitPayForBlob`.
`app/test/.../max_total_blob_size_test.go`	Removed imports and constants, renamed and refocused blob size test function.
`app/test/.../square_size_test.go`	Removed "_Flaky" suffix from `TestSquareSizeUpperBound` function.
`app/test/unit_test.go`	Introduced `app_test` package and defined `kibibyte` and `mebibyte` constants.
`pkg/square/.../square_test.go`	Renamed test function, removed `rand` variable, added new assertion.
`test/util/testnode/.../accounts.go`	Introduced `RandomAccounts` function in new `testnode` package.
`test/util/testnode/.../accounts_test.go`	Added new test function `TestRandomAccounts`.
`test/util/testnode/.../full_node_test.go`	Renamed test functions, updated constants and arguments, removed block height wait check.
`test/util/testnode/.../node_interaction_api.go`	Modified `FillBlock` method to accept a single account string.
`x/blob/client/.../integration_test.go`	Reorganized setup, refactored and removed test cases, added short mode check.
`.github/workflows/lint.yml`	Added `--verbose` flag to golangci-lint action args.

Assessment against linked issues

Objective	Addressed	Explanation
Investigate and address CI test failure in `TestMaxTotalBlobSizeSuite/TestSubmitPayForBlob_blobSizes` (#2672)	❓	The renaming and refocusing of the test function suggest an attempt to address flakiness, but without specific details on the failure cause or resolution, it's unclear if the issue is fully resolved.
Ensure `TestMaxTotalBlobSizeSuite` and `TestSubmitPayForBlob_blobSizes` are stable in CI (#2672)	❓	Changes imply an effort to stabilize the tests, but without CI results or explicit mention of the flakiness fix, it's uncertain if the stability has been achieved.
Remove `FailNow` on a parent test in subtest `2_mebibyte_blob` to prevent panic/runtime.Goexit (#2672)	✅	The removal of certain test cases and refocusing on specific blob sizes may indicate that the problematic test structure has been addressed.
Match expected value `0x2b82` with actual value `0x20` in tests (#2672)	❌	There is no direct evidence in the summary provided that this specific mismatch issue has been addressed.

Please note that the assessment is based on the provided summaries and without access to the full context or the ability to review the actual code changes, the assessment may not fully reflect the state of the codebase.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

b/c it is redundant with TestSubmitPayForBlob

app/test/integration_test.go

rootulp · 2023-11-25T02:41:39Z

app/test/integration_test.go

+	Kibibyte = 1024
+	Mebibyte = 1024 * Kibibyte
+)
+
 func TestIntegrationTestSuite(t *testing.T) {


passing

$ go test -count=2 . -run "TestIntegrationTestSuite" ok github.com/celestiaorg/celestia-app/app/test 50.124s

rootulp · 2023-11-25T03:00:35Z

app/test/max_total_blob_size_test.go

-func (s *MaxTotalBlobSizeSuite) TestSubmitPayForBlob_blobSizes() {
+// TestErrTotalBlobSizeTooLarge verifies that submitting a 2 MiB blob hits
+// ErrTotalBlobSizeTooLarge.
+func (s *MaxTotalBlobSizeSuite) TestErrTotalBlobSizeTooLarge() {


seems less flaky

$ go test -count=5 . -run "TestMaxTotalBlobSizeSuite" ok github.com/celestiaorg/celestia-app/app/test 31.621s

rootulp · 2023-11-25T03:10:23Z

app/test/square_size_test.go

@@ -60,8 +60,7 @@ func (s *SquareSizeIntegrationTest) SetupSuite() {

 // TestSquareSizeUpperBound sets the app's params to specific sizes, then fills the
 // block with spam txs to measure that the desired max is getting hit
-// The "_Flaky" suffix indicates that the test may fail non-deterministically especially when executed in CI.
-func (s *SquareSizeIntegrationTest) TestSquareSizeUpperBound_Flaky() {
+func (s *SquareSizeIntegrationTest) TestSquareSizeUpperBound() {


can't get it to fail locally but we should explore making this test faster

$ go test -count=1 . -run "TestSquareSizeIntegrationTest" ok github.com/celestiaorg/celestia-app/app/test 106.361s

rootulp · 2023-11-25T03:20:11Z

x/blob/client/testutil/integration_test.go

@@ -182,7 +177,9 @@ func (s *IntegrationTestSuite) TestSubmitPayForBlob() {
 	}
 }

-// The "_Flaky" suffix indicates that the test may fail non-deterministically especially when executed in CI.
-func TestIntegrationTestSuite_Flaky(t *testing.T) {
+func TestIntegrationTestSuite(t *testing.T) {


Can't repro locally

$ go test ./... -count=3 -run "TestIntegrationTestSuite" ok github.com/celestiaorg/celestia-app/x/blob/client/testutil 64.982s

we could get this to go way faster if we refactored it to not use the cosmos-sdk's network code #829

test/util/testnode/accounts_test.go

test/util/testnode/node_interaction_api.go

https://stackoverflow.com/questions/74239074/context-todo-or-context-background-which-one-should-i-prefer context.Background() seems preferable for this test where the surrounding code is not going to be refactored to accept a parent context

app/test/integration_test.go

app/test/max_total_blob_size_test.go

test/util/testnode/node_interaction_api.go

x/blob/client/testutil/integration_test.go

evan-forbes · 2023-11-27T20:48:49Z

x/blob/client/testutil/integration_test.go

@@ -182,7 +177,9 @@ func (s *IntegrationTestSuite) TestSubmitPayForBlob() {
 	}
 }

-// The "_Flaky" suffix indicates that the test may fail non-deterministically especially when executed in CI.
-func TestIntegrationTestSuite_Flaky(t *testing.T) {
+func TestIntegrationTestSuite(t *testing.T) {


we could get this to go way faster if we refactored it to not use the cosmos-sdk's network code #829

x/blob/client/testutil/integration_test.go

Duplicative of `TestSubmitPayForBlob` in signer_test.go

app/test/integration_test.go

staheri14

LGTM!

staheri14 · 2023-11-28T00:41:29Z

app/test/integration_test.go

 		return blobfactory.RandBlobTxsWithAccounts(
 			s.ecfg,
 			tmrand.NewRand(),
 			s.cctx.Keyring,
 			c.GRPCClient,
-			950000,
+			600*kibibyte,


[Question] Curious to know why this change was needed?

This change wasn't strictly needed. I thought it was misleading that the variable name stated 1Mb when the generator created transactions that weren't 1Mb. Opted to change the variable name and the size of txs created.

staheri14 · 2023-11-28T00:42:43Z

app/test/integration_test.go

@@ -80,47 +72,41 @@ func (s *IntegrationTestSuite) SetupSuite() {
 func (s *IntegrationTestSuite) TestMaxBlockSize() {
 	t := s.T()

-	// tendermint's default tx size limit is 1Mb, so we get close to that


[Question] Shouldn't this be revised instead of removed?

The comment is no longer accurate b/c now this tx generator creates transactions that are ~600 KiB

staheri14 · 2023-11-28T00:44:04Z

app/test/integration_test.go

 			3,
 			false,
 			s.accounts[20:40],
 		)
 	}

-	// Generate 80 randomly sized txs (max size == 50 KB). Generate these


[Question] Are these comments removed intentionally?

Yes.

Generate these transactions using some of the same accounts as the previous generator to ensure that the sequence number is being utilized correctly in blob txs

was incorrect because this tx generator used accounts 40 - 120 which does not overlap with the previous tx generator (20 - 40). So I deleted the misleading comment.

staheri14 · 2023-11-28T00:47:49Z

x/blob/client/testutil/integration_test.go

@@ -89,58 +81,14 @@ func (s *IntegrationTestSuite) TestSubmitPayForBlob() {
 			expectedCode: 0,
 			respType:     &sdk.TxResponse{},
 		},
-		{
-			name: "unsupported share version",


[Question] Mind elaborating why these tests cases were needed to be removed? how did they contribute to the test flakiness?

they didn't contribute to flakiness but they were removed b/c they don't strictly need to be included in this integration test. Instead these cases should be covered by unit tests

staheri14 · 2023-11-28T00:51:04Z

test/util/testnode/full_node_test.go

@@ -127,7 +123,7 @@ func (s *IntegrationTestSuite) Test_FillBlock_InvalidSquareSizeError() {

 	for _, tc := range tests {
 		s.Run(tc.name, func() {
-			_, actualErr := s.cctx.FillBlock(tc.squareSize, s.accounts, flags.BroadcastAsync)
+			_, actualErr := s.cctx.FillBlock(tc.squareSize, s.accounts[2], flags.BroadcastAsync)


[Optional] I suppose s.accounts points to a single account, right? then perhaps we could get rid of the plural s at the end.

s.accounts is still a slice with multiple accounts. This line just uses one of those accounts.

staheri14 · 2023-11-28T00:53:31Z

test/util/testnode/full_node_test.go

 	var params *coretypes.ResultConsensusParams
 	// this query can be flaky with fast block times, so we repeat it multiple
 	// times in attempt to decrease flakiness
-	for i := 0; i < 40; i++ {
-		params, err = s.cctx.Client.ConsensusParams(context.TODO(), nil)
+	for i := 0; i < 100; i++ {


[Question] Mind explaining why it has been increased to 100?

to decrease flakiness

staheri14 · 2023-11-28T00:54:44Z

test/util/testnode/full_node_test.go

@@ -19,7 +19,14 @@ import (
 	coretypes "github.com/tendermint/tendermint/rpc/core/types"
 )

+const (
+	kibibyte = 1024


[nit]

Suggested change

kibibyte = 1024

kibibyte = 1024 // bytes

I really want to commit this suggestion but I'm going to skip it b/c accepting it will require re-approvals which takes too much time

staheri14 · 2023-11-28T01:00:15Z

app/test/unit_test.go

+	kibibyte = 1024
+	mebibyte = 1024 * kibibyte


[Suggestion] For improved reusability and maintainability, all such unit converter constants could be consolidated into a single location.

Created #2873

test: attempt to unflake max total blob size

c3758ff

rootulp self-assigned this Nov 10, 2023

rootulp added 13 commits November 10, 2023 14:15

refactor: mustNewBlob -> newBlobWithSize

d29f4b3

refactor: setup accounts

841cc14

refactor: TestMaxBlockSize

9f7a4a8

refactor: remove redundant continue

c0e908d

refactor: remove unnecessary verification

cb7d883

refactor: collapse vertically

9123e69

refactor: rename to ExtendBlockTest

c08a962

refactor: use sizes

18a9632

refactor: extract RandomAccounts()

bf93ad2

refactor: rename tests

d17de92

refactor: rename test

50e3282

refactor: remove _flaky

2a2cd22

test: attempt to differentiate accounts per test

0369f21

Potential solution for https://github.com/celestiaorg/celestia-app/actions/runs/6827269518/job/18569018708#step:4:49

rootulp changed the title ~~test: attempt to unflake max total blob size~~ test: attempt to resolve flakes Nov 10, 2023

test: remove _flaky suffix

4ed02a5

jaybxyz mentioned this pull request Nov 14, 2023

fix: register gRPC Gateway routes #2838

Merged

5 tasks

rootulp commented Nov 25, 2023

View reviewed changes

rootulp added 2 commits November 24, 2023 21:12

refactor: units

278fc27

test: remove redundant test cases

cf47f2e

rootulp added 2 commits November 24, 2023 21:35

refactor: use units

d625910

test: delete submit PFB

e3a9a23

b/c it is redundant with TestSubmitPayForBlob

rootulp commented Nov 25, 2023

View reviewed changes

app/test/integration_test.go Show resolved Hide resolved

rootulp commented Nov 25, 2023

View reviewed changes

app/test/integration_test.go Show resolved Hide resolved

rootulp commented Nov 25, 2023

View reviewed changes

refactor: extract shared units

0a3eb0a

rootulp commented Nov 25, 2023

View reviewed changes

rootulp added 2 commits November 24, 2023 22:16

remove redundant tests

ee5d427

refactor: unexport unit

2c0273f

rootulp commented Nov 25, 2023

View reviewed changes

Merge branch 'main' into rp/refactor-test

9b3db74

rootulp marked this pull request as ready for review November 25, 2023 03:22

rootulp requested review from evan-forbes and cmwaters as code owners November 25, 2023 03:22

coderabbitai bot reviewed Nov 25, 2023

View reviewed changes

test/util/testnode/accounts_test.go Show resolved Hide resolved

test/util/testnode/node_interaction_api.go Show resolved Hide resolved

celestiaorg deleted a comment from codecov-commenter Nov 25, 2023

ci: debug lint failure

39ea848

celestia-bot requested a review from a team November 25, 2023 03:37

attempt to fix lint

39148f6

rootulp requested a review from staheri14 November 25, 2023 03:43

chore: prefer context.Background()

8da4fa5

https://stackoverflow.com/questions/74239074/context-todo-or-context-background-which-one-should-i-prefer context.Background() seems preferable for this test where the surrounding code is not going to be refactored to accept a parent context

evan-forbes previously approved these changes Nov 27, 2023

View reviewed changes

rootulp added 3 commits November 27, 2023 16:05

refactor: rename test

1447745

refactor: remove ErrTxTooLarge test case

f427908

refactor: delete test entirely

efcb52d

Duplicative of `TestSubmitPayForBlob` in signer_test.go

rootulp dismissed evan-forbes’s stale review via efcb52d November 27, 2023 21:10

celestia-bot requested a review from a team November 27, 2023 21:10

coderabbitai bot reviewed Nov 27, 2023

View reviewed changes

app/test/integration_test.go Show resolved Hide resolved

app/test/integration_test.go Show resolved Hide resolved

app/test/integration_test.go Show resolved Hide resolved

app/test/integration_test.go Show resolved Hide resolved

test: new test case for square construction

3e0c902

rootulp changed the title ~~test: attempt to resolve flakes~~ test!: attempt to resolve flakes Nov 27, 2023

revert: network -> net

4a9d8e5

evan-forbes approved these changes Nov 27, 2023

View reviewed changes

staheri14 approved these changes Nov 28, 2023

View reviewed changes

rootulp mentioned this pull request Nov 28, 2023

Consider consolidating constants for data sizes (i.e. kibibyte, mebibyte) #2873

Closed

rootulp merged commit 54e8936 into celestiaorg:main Nov 28, 2023
30 checks passed

rootulp deleted the rp/refactor-test branch November 28, 2023 16:58

rootulp mentioned this pull request Nov 30, 2023

EPIC: flakey CI #2827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test!: attempt to resolve flakes #2834

test!: attempt to resolve flakes #2834

rootulp commented Nov 10, 2023 •

edited

Loading

rootulp Nov 25, 2023

coderabbitai bot commented Nov 25, 2023 •

edited

Loading

Rate Limit Exceeded

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

rootulp Nov 25, 2023

rootulp Nov 25, 2023

rootulp Nov 25, 2023

rootulp Nov 25, 2023

evan-forbes Nov 27, 2023

evan-forbes Nov 27, 2023

staheri14 left a comment

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

staheri14 Nov 28, 2023

rootulp Nov 28, 2023

test!: attempt to resolve flakes #2834

test!: attempt to resolve flakes #2834

Conversation

rootulp commented Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

coderabbitai bot commented Nov 25, 2023 • edited Loading

Rate Limit Exceeded

Walkthrough

Changes

Assessment against linked issues

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp commented Nov 10, 2023 •

edited

Loading

coderabbitai bot commented Nov 25, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)